Treat frustum changes as camera changes in Scene#render #6821

lukesanantonio · 2018-07-19T20:18:53Z

Before this change, the user could update the frustum but not the camera by resizing the viewer. The Scene would continue to render even when requestRenderMode was enabled because it recognized the frustum had changed, but would not update the cached camera to mark the change as handled. This commit changes cameraEqual (in Scene.js) to take into account the camera frustum.

Also adds equalsEpsilon methods to the *Frustum types to make this work.

Fixes #6812

cesium-concierge · 2018-07-19T20:18:54Z

Thanks for the pull request @lukesanantonio!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

Before this change, the user could update the frustum but not the camera by resizing the viewer. The Scene would continue to render even when requestRenderMode was enabled because it recognized the frustum had changed, but would not update the cached camera to mark the change as handled. This commit changes cameraEqual (in Scene.js) to take into account the camera frustum. Fixes CesiumGS#6812

mramato · 2018-07-19T20:49:17Z

Thanks for tracking this down @lukesanantonio! @ggetz can you review? @bagnell are you okay with adding these methods to Frustum?

Fixes #6812

Mention this explicitly in CHANGES and link to this PR or the original issue. Look at other recent bullets in the markdown for examples

ggetz

I can confirm this fixes the issue! Thanks @lukesanantonio, just a few comments.

ggetz · 2018-07-19T21:24:04Z

Source/Core/OrthographicOffCenterFrustum.js

+                CesiumMath.equalsEpsilon(this.top, other.top, relativeEpsilon, absoluteEpsilon) &&
+                CesiumMath.equalsEpsilon(this.bottom, other.bottom, relativeEpsilon, absoluteEpsilon) &&
+                CesiumMath.equalsEpsilon(this.near, other.near, relativeEpsilon, absoluteEpsilon) &&
+                CesiumMath.equalsEpsilon(this.far, other.far, relativeEpsilon, absoluteEpsilon));


@bagnell Does it make sense to use the same epsilon for all these values?

ggetz · 2018-07-19T21:24:53Z

Source/Core/OrthographicFrustum.js

+                CesiumMath.equalsEpsilon(this.aspectRatio, other.aspectRatio, relativeEpsilon, absoluteEpsilon) &&
+                CesiumMath.equalsEpsilon(this.near, other.near, relativeEpsilon, absoluteEpsilon) &&
+                CesiumMath.equalsEpsilon(this.far, other.far, relativeEpsilon, absoluteEpsilon) &&
+                this._offCenterFrustum.equalsEpsilon(other._offCenterFrustum, relativeEpsilon, absoluteEpsilon));


I think all these checks are redundant- After updating, you should be able to just check this._offCenterFrustum.equalsEpsilon(other._offCenterFrustum, relativeEpsilon, absoluteEpsilon));

It seems like the *OffCenter* variants don't check fov,width, or aspectRatio. Is that still alright?

Oh I didn't realize you only meant the ones you highlighted. near and far checks are def redundant

ggetz · 2018-07-19T21:26:14Z

Source/Core/OrthographicFrustum.js

+     * <code>true</code> if they pass an absolute or relative tolerance test,
+     * <code>false</code> otherwise.
+     *
+     * @param {OrthographicFrustum} [other] The right hand side OrthographicFrustum.


I think this should be required. In practice, it doesn't really make sense to use this function with the first parameter undefined and the second parameter a value. Same for all of these functions.

That's fair, is there a way to mark this in the docs or should I just remove the defined(other) part of the condition?

Just update the docs by removing the brackets, [].

Ha! I didn't realize those brackets mark them optional. Will fix

ggetz · 2018-07-19T21:27:02Z

Source/Core/PerspectiveFrustum.js

+                CesiumMath.equalsEpsilon(this.aspectRatio, other.aspectRatio, relativeEpsilon, absoluteEpsilon) &&
+                CesiumMath.equalsEpsilon(this.near, other.near, relativeEpsilon, absoluteEpsilon) &&
+                CesiumMath.equalsEpsilon(this.far, other.far, relativeEpsilon, absoluteEpsilon) &&
+                this._offCenterFrustum.equalsEpsilon(other._offCenterFrustum, relativeEpsilon, absoluteEpsilon));


Same comment here as for OrthographicFrustum.js

ggetz · 2018-07-19T21:30:16Z

Source/Scene/Scene.js

        var cameraChanged = checkForCameraUpdates(this);
-        var shouldRender = !this.requestRenderMode || this._renderRequested || cameraChanged || this._frustumChanged || this._logDepthBufferDirty || (this.mode === SceneMode.MORPHING);


Is this the only place scene._frustumChanged is used? If so, we can probably remove it entirely.

It's used on Line 1513. It didn't look like cameraChanged (or something like it) could be a drop-in replacement so I didn't change it.

Yes, that's fine to leave.

ggetz · 2018-07-19T21:31:24Z

Source/Scene/Scene.js

@@ -1488,7 +1488,8 @@ define([
               Cartesian3.equalsEpsilon(camera0.direction, camera1.direction, epsilon) &&
               Cartesian3.equalsEpsilon(camera0.up, camera1.up, epsilon) &&
               Cartesian3.equalsEpsilon(camera0.right, camera1.right, epsilon) &&
-               Matrix4.equalsEpsilon(camera0.transform, camera1.transform, epsilon);
+               Matrix4.equalsEpsilon(camera0.transform, camera1.transform, epsilon) &&
+               camera0.frustum.equalsEpsilon(camera1.frustum, epsilon);


Add (or modify) a test to the Scene request render specs to test if this is being accounted for.

I'm having trouble getting the test to fail on master. I'll try again tomorrow but do you have any tips / ideas?

Take a look at the other requestRender tests, if you haven't already.

Got it, the newest commit has a test that failed before and passes with these changes.

hpinkos · 2018-07-24T19:56:22Z

@lukesanantonio we want to try to get this in for the next release, so try to address these comments when you have a minute. Thanks!

lukesanantonio · 2018-07-26T05:05:50Z

Sorry for the delay, made some changes but I still need some help on a few things @ggetz @hpinkos

lukesanantonio · 2018-07-27T20:59:51Z

Okay I think this is all good @ggetz

ggetz · 2018-07-27T21:27:18Z

Awesome, thanks @lukesanantonio. Please also merge in master when you get the chance.

ggetz · 2018-07-30T13:31:20Z

Looks good, thanks @lukesanantonio !

lukesanantonio force-pushed the frustum-render-fix branch from 6a7a739 to 39da67d Compare July 19, 2018 20:40

Luke San Antonio Bialecki added 3 commits July 19, 2018 16:41

Add equalsEpsilon method to all Frustum types

c3bb2be

Update CHANGES.md

c7a1b0c

lukesanantonio force-pushed the frustum-render-fix branch from 39da67d to 3fa9b0e Compare July 19, 2018 20:43

Update CHANGES.md

f626f31

ggetz self-requested a review July 20, 2018 18:33

ggetz requested changes Jul 20, 2018

View reviewed changes

hpinkos added the priority - next release label Jul 24, 2018

lukesanantonio added 2 commits July 26, 2018 00:45

Remove redundant checks in *Frustum comparison methods

a041bbf

Merge branch 'master' into frustum-render-fix

0b00e1a

lukesanantonio added 2 commits July 26, 2018 11:33

Do not focus on test in OrthographicFrustumSpec

198cd4d

Add regression test for issue CesiumGS#6812

3f08132

lukesanantonio force-pushed the frustum-render-fix branch from 34995ba to 3f08132 Compare July 26, 2018 15:52

Mark first parameter as required for each *Frustum equalsEpsilon method

b7c1e30

Merge remote-tracking branch 'upstream/master' into frustum-render-fix

564ef27

ggetz merged commit 6a39ce5 into CesiumGS:master Jul 30, 2018

lukesanantonio deleted the frustum-render-fix branch August 17, 2018 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat frustum changes as camera changes in Scene#render #6821

Treat frustum changes as camera changes in Scene#render #6821

lukesanantonio commented Jul 19, 2018

cesium-concierge commented Jul 19, 2018 •

edited by ggetz

Loading

mramato commented Jul 19, 2018

ggetz left a comment

ggetz Jul 19, 2018

ggetz Jul 19, 2018

lukesanantonio Jul 26, 2018

lukesanantonio Jul 26, 2018

ggetz Jul 19, 2018

lukesanantonio Jul 26, 2018

ggetz Jul 27, 2018

lukesanantonio Jul 27, 2018

ggetz Jul 19, 2018

ggetz Jul 19, 2018

lukesanantonio Jul 26, 2018

ggetz Jul 27, 2018

lukesanantonio Jul 27, 2018

ggetz Jul 19, 2018

lukesanantonio Jul 26, 2018

ggetz Jul 27, 2018

lukesanantonio Jul 27, 2018

hpinkos commented Jul 24, 2018

lukesanantonio commented Jul 26, 2018

lukesanantonio commented Jul 27, 2018

ggetz commented Jul 27, 2018

ggetz commented Jul 30, 2018

		var cameraChanged = checkForCameraUpdates(this);
		var shouldRender = !this.requestRenderMode \|\| this._renderRequested \|\| cameraChanged \|\| this._frustumChanged \|\| this._logDepthBufferDirty \|\| (this.mode === SceneMode.MORPHING);

Treat frustum changes as camera changes in Scene#render #6821

Treat frustum changes as camera changes in Scene#render #6821

Conversation

lukesanantonio commented Jul 19, 2018

cesium-concierge commented Jul 19, 2018 • edited by ggetz Loading

mramato commented Jul 19, 2018

ggetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpinkos commented Jul 24, 2018

lukesanantonio commented Jul 26, 2018

lukesanantonio commented Jul 27, 2018

ggetz commented Jul 27, 2018

ggetz commented Jul 30, 2018

cesium-concierge commented Jul 19, 2018 •

edited by ggetz

Loading